-
-
Notifications
You must be signed in to change notification settings - Fork 784
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NW6/Fidaa Bashir/HTML-CSS/bikes-for-refugees/Week 1 #475
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cyf-bfr ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
This is lovely work. My sole complint is that the nav bar is in the wrong position compared to the image. Can you have a crack at moving it into that position?
index.html
Outdated
<img src="./images/bikes-for-refugees_logo.jpg" alt="Logo for Bikes for Refugees Scotland" class="header__logo" /> | ||
</a> | ||
|
||
<nav> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good to see you diving straight in and doing the sematic HTML work Looking good!
<h2 class="heading-underline">Upcoming events</h2> | ||
|
||
<article class="article"> | ||
<img class="article__thumbnail" src="./images/spring-fundraisers_thumbnail.jpg" alt="fundraisers" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see objective 2 achieved.
@@ -94,8 +103,36 @@ p { | |||
|
|||
|
|||
/* Buttons */ | |||
button { | |||
border-radius: 0.3rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really love the way you've made this style, it's professional looking and you've used CSS varibles and classes to avoid reuse of coe. Objective 3 complete!
background-color: var(--grey-light); | ||
padding: 6rem 3rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, good that you used rem over px - this was a CYF best practice last I heard
} | ||
|
||
.navigation__item:hover { | ||
background-color: #c05326; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did the stretch goals to! Nice! Should also have been a var maybe, though this a matter of your own prefrences wher're you're only using it once
Thank you @Ara225 for your code review. I've moved the nav bar to the right position as requested :) |
Brillant looking good :) |
No description provided.